-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix #29936, precompile should not assume UnionAlls have stable addresses #31047
Conversation
This appears to assert that |
The problem here is that there is a special serialization for Another way to look at it is that if |
Right, I understand that's why this hides the bug. But just pointing out that it seems to potentially indicate a deeper issue, where we assume that TypeVar identity will be correctly maintained, but doesn't. |
This is an example of what this PR could cause to fail (if it appeared in user code): diff --git a/test/precompile.jl b/test/precompile.jl
index ed93b60bfd..d3417bfc8f 100644
--- a/test/precompile.jl
+++ b/test/precompile.jl
@@ -135,6 +135,8 @@ try
const x28297 = Result(missing)
+ const d28297a = UnionAll(Dict.var, UnionAll(Dict.body.var, Dict.body.body)) # === Dict
+ const d28297b = UnionAll(Dict.body.var, UnionAll(Dict.var, Dict.body.body)) # === Dict{K,V} where K where V !== Dict
# issue #28998
const x28998 = [missing, 2, missing, 6, missing,
@@ -186,6 +188,8 @@ try
@test Foo.abigint_x::BigInt + 1 == big"125"
@test Foo.x28297.result === missing
+ @show Foo.d28297a # === Dict
+ @show Foo.d28297b # === Dict
@test Foo.x28998[end] == 6
end |
typejoin
can re-form a type likeVal{x} where x
, re-usingVal.var
andVal.body
but allocating a new UnionAll. However,jl_serialize_datatype
compares the address of the inner type (Val.body
) while the code for serializing UnionAll compares the address of the UnionAll itself, leading to a possible inconsistency. This makes them both use the inner type, which has a stable address.Backportable like whoa.